Skip to content

First implementation #1

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 3 commits into from
Dec 21, 2023
Merged

First implementation #1

merged 3 commits into from
Dec 21, 2023

Conversation

sfe-SparkFro
Copy link
Collaborator

No description provided.

SFE-Brudnerd
SFE-Brudnerd suggested changes Dec 19, 2023
View reviewed changes
Copy link

@SFE-Brudnerd SFE-Brudnerd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall good, needs a few changes.

@SFE-Brudnerd
Copy link

One other thing, you should add an address getter since you have a setter.

@sfe-SparkFro
Code review updates
6226419 
Change constants to Hungarian notation
Move _theBus initializer into header
Change changeAddress() to take a `const uint8_t&`
Remove under read check, since that will be caught by previous check of err
Add address getter
SFE-Brudnerd
SFE-Brudnerd approved these changes Dec 20, 2023
View reviewed changes
Copy link

@SFE-Brudnerd SFE-Brudnerd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

edspark
edspark approved these changes Dec 20, 2023
View reviewed changes
Copy link
Contributor

@edspark edspark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing to add, this looks great Dryw.

MadisonC-SparkFun
MadisonC-SparkFun approved these changes Dec 21, 2023
View reviewed changes
Copy link

@MadisonC-SparkFun MadisonC-SparkFun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks great! Nice job :)

gigapod
gigapod reviewed Dec 21, 2023
View reviewed changes
Copy link
Member

@gigapod gigapod left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks great.

What I suggested are easy tweaks - for readability and future understanding.

@sfe-SparkFro
Code review updates
a6a3c18 
In computing number of addresses, divide by size of elements
Check err against OK constant
@sfe-SparkFro sfe-SparkFro merged commit edc5e99 into main Dec 21, 2023
@sfe-SparkFro sfe-SparkFro deleted the v1.0.0 branch December 21, 2023 21:50
@sfe-SparkFro
Copy link
Collaborator Author

Thanks all! Library has been published and merged into Arduino's library registry: arduino/library-registry#3782

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gigapod gigapod gigapod left review comments

@edspark edspark edspark approved these changes

@SFE-Brudnerd SFE-Brudnerd SFE-Brudnerd approved these changes

@MadisonC-SparkFun MadisonC-SparkFun MadisonC-SparkFun approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@sfe-SparkFro @SFE-Brudnerd @gigapod @edspark @MadisonC-SparkFun